-
Notifications
You must be signed in to change notification settings - Fork 127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Development/websocket #1794
Development/websocket #1794
Conversation
…m item - Rename 'WEBSERVER' to 'WEBSERVICE' as both client and server utilize state information - Unsollicited 'Pong' may act as a heart beat
…ructure are not yet contructed, and, improve SSL read and write use - SSL read and write methods should only be used if the handshake is successfull - Do not return error values for SSL read and write methods as number of read or written bytes
…opened but context has not been contructed.
…ly) propagate to SSL structure after construction. - Context should exist - Options should be set
@@ -105,10 +105,14 @@ namespace Crypto { | |||
|
|||
// Signal a state change, Opened, Closed or Accepted | |||
void StateChange() override { | |||
if (IsOpen() && _context == nullptr) { // Initialize may not yet have happened |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By design the ASSERT(_context != nullptr) and lets add ASSERT(_ssl != nullptr) could be added here, cause if either of these are a nullptr, the Initialize should have signaled that by sending out an error != CORE_ERROR_NONE and than the statechange will never be called!
So I consider this unusefull additional code and think that it is a design flaw if a socket could be "open" but it was not Initialized(), see:
Thunder/Source/core/SocketPort.cpp
Line 516 in fe2b0a5
if ((m_Socket != INVALID_SOCKET) && (Initialize() == Core::ERROR_NONE)) { |
if (IsOpen() == true) { | ||
int result; | ||
|
||
if (IsOpen() == true && _ssl != nullptr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the SecureSocketPort::Handler::Initialize() an error is returned if _ssl was not constructed. Please code defensively, just ASSERT here on an (_ssl != nullptr);
SSL_set_tlsext_host_name(static_cast<SSL*>(_ssl), RemoteNode().HostName().c_str()); | ||
result = SSL_connect(static_cast<SSL*>(_ssl)); | ||
if (result == 1) { | ||
int result{0}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stick to coding as done every where int result = 0;
return (SSL_write(static_cast<SSL*>(_ssl), buffer, length)); | ||
int32_t result = 0; | ||
|
||
if (_handShaking == CONNECTED) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was/is a design flaw, if someone would write already while there is no "OPEN"/"CONNECTED state, it might end up in a busy-loop. Suggest an ASSERT( _handShaking == CONNECTED); Programitically the user of the socket has to wait till the StateChange -> Open has ocured before the Write can be used!!
|
||
if (_handShaking != CONNECTED) { | ||
const_cast<Handler&>(*this).Update(); | ||
if (_handShaking == CONNECTED) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be interesting to test if an ASSERT(handshaking == CONNECTED); fires. If so I guess the Update is usefull for the system to update its information if it was not CONNECTED (something that now is turned off).
If the ASSERT does not fire, lets clean up and do not call Update() and make the code more lean and mean!
|
||
if ((_context = SSL_CTX_new(TLS_method())) != nullptr) { | ||
if ( // Returns bit-mask after adding options | ||
((SSL_CTX_set_options(static_cast<SSL_CTX*>(_context), SSL_OP_ALL | SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3) & (SSL_OP_ALL | SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3)) == (SSL_OP_ALL | SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SSL_CTX_set_options contruction feels a bit like you are checking the openSSL implementation if it does what you are asking it to do. The manual does not give any restrictions on mutal exclusive options. So since we do not actively want to check third party libraries in Production or Release I suggest putting this check in an ASSERT if you want to check OpenSSL
@msieben as I worked through your test code to reproduce the reason why you added the Initialize() in the StateChange() to find the bug mentioned in the PR above, I saw that the code you worte for the testserver/client is not thread safe! std::vector is not thread safe, you are using it (also in your test) in a multithreaded way, so be carefull! |
Test code:
and the test:
|
@pwielders thanks for the feedback though it was originally not intended to be merged, hence, the checks instead of the ASSERTs to enable a continuing workflow. Nevertheless, I appreciate the refreshing hints and advices. The underlying issue as presented in #1795 should nullify al lot of the commits that comprise this PR and present a better base to continue efforts. |
No description provided.